Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Keyboard Shortcuts #191

Merged
merged 6 commits into from
Jan 5, 2018
Merged

Add Keyboard Shortcuts #191

merged 6 commits into from
Jan 5, 2018

Conversation

wgranger
Copy link
Contributor

@wgranger wgranger commented Nov 28, 2017 •
edited
Loading

This includes four keyboard shortcuts as well as a descriptive popup to notify the user what the shortcuts are.

There are also some line changes to make items agree with the linter (the bulk of the changes)

Closes #172

shaunanoordin
shaunanoordin requested changes Nov 29, 2017
View reviewed changes
Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

This PR adds keyboard shortcuts to the app.

  • Esc when transcribing - closes Selected Annotation window.
  • Ctrl+Enter when transcribing - saves transcription, closes Selected Annotation window.
  • Ctrl+M - Toggle Previous Marks.
  • Ctrl+N - Toggle Annotate/Panning mode.

Basic keyboard functionality is on point - e.g. I like that removeEventListener wasn't forgotten, and that we're going with onKeyUp instead of onKeyDown. (onClick would have been acceptable too.)

There are some cases that the code doesn't account for yet, though.

Comments

  • Esc when transcribing - This should replicate the 'Cancel' button, and in most cases, it does.

    • However, the Cancel button does one extra thing - if a newly-created Annotation (not previous text) is Cancelled, it is also Deleted. The Esc button does not replicate this yet.

  • Ctrl+Enter when transcribing - This replicates what the 'Done' button does, but doesn't account for the word count logic.

    • Currently I can submit invalid (word count < number of dots - 1) transcriptions by using Ctrl+Enter

  • Ctrl+N - I think this opens a new window on Win10+Firefox. I need to double check this.

  • Ctrl+M - I have no known issues with this, but I should double check with the windows systems.

screen shot 2017-11-29 at 15 09 09
Also, I'm not sure if I'm missing some styles - I'll try refreshing the system and checking again.

Recommended solutions

  • For the Esc issue: use this.cancelAnnotation() instead.
  • For the Enter issue: use wordCountMatchesDots && this.saveText - but if you want to go an extra mile, you'll want to signal to the user that the Ctrl+Enter failed because of an invalid word count.
  • For the Ctrl+N issue... pick another letter, I guess?

Status

Main functionality confirmed, but changes are requested to handle the weird cases.

@shaunanoordin
Copy link
Member

Confirmed: Ctrl+N opens a new window on most browsers (Chrome, FF, IE11) on Windows machines.

Ctrl+M seems safe?

Here's a list of common browser shortcuts - https://www.howtogeek.com/114518/47-keyboard-shortcuts-that-work-in-all-web-browsers/ - so it should give us an advantage in figuring out what to avoid.

@shaunanoordin
Copy link
Member

DERP! I forgot that Esc is a common key for stopping the page from loading - but that's fine and I'm strongly in favour of keeping the Esc as is. The Esc key is a very, very sensible, multipurpose key that's OK to be overloaded.

Besides, once the website has loaded, the Esc key doesn't stop any further data fetches, so it looks like we're good.

@wgranger
Copy link
Contributor Author

Thanks for the thorough review on this. You caught some items that would've caused issues in the future. I've changed switching between annotate/navigate to ctrl + a. A is for 'annotate.'

@snblickhan
Copy link
Collaborator

Priority for beta 2

@shaunanoordin
Copy link
Member

This PR may need to be rebased after #198 is merged; IIRC there are some notable changes to... SelectedAnnotation, I think, that reflects how we no longer need to check for word count. I'll look into this one 198 is out of the way.

Also, we need another option for the Annotate shortcut unfortunately, as Ctrl+A in Windows means "select all", and will cause the whole page to be highlighted.

@wgranger
Copy link
Contributor Author

wgranger commented Jan 4, 2018 •
edited
Loading

About the make the changes mentioned above. Also @beckyrother did we at some point discuss (in Slack possibly?) using cmd in the case of a Mac? Or also ctrl? Both? There is a metakey as mentioned in the link.
(https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/metaKey)
Actually, on review of the above, it seems these meta keys already have shortcuts applied to them inherent in the OS.

@beckyrother
Copy link

We had talked about simply using the letter keys without a modifier (so a for annotate, etc). Looking at gmail as a model.

Here's a good Medium post about single-letter keyboard shortcuts, for reference: https://medium.com/@sashika/j-k-or-how-to-choose-keyboard-shortcuts-for-web-applications-a7c3b7b408ee

@shaunanoordin
Copy link
Member

Now that #198 has been merged, I'm looking at conflict resolution at the moment - let's see if we can do this without a rebase.

@shaunanoordin
Copy link
Member

Testing ongoing: I need to see what 'Enter' does when the text is empty. (I can't think of other 'invalid' states.)

@beckyrother
Copy link

Enter shouldn't do anything when the text is empty. Maybe 'esc' closes the box?

@shaunanoordin
Copy link
Member

@beckyrother Right, so I just merged the latest changes from the master (the Single Line Annotation stuff) to this PR and everything's compatible. Let's talk expected vs actual behaviour. 👌

Right now, when the text is empty and you click Done or Press Enter, the app goes "Look, I'm assuming you're saying the Line of Text is empty (no words), so let's just delete that Line."

Full details:

  • When the user marks the start and end of a new Line of Text, the Transcription dialog auto-opens...
    • If the user types nothing "" and clicks Close/presses Esc, the new Line is deleted.
    • If the user types nothing "" and clicks Done/presses Enter, the new Line is deleted. 👀
    • If the user types something "tralala" and clicks Close/presses Esc, the new Line is deleted. 👀
    • If the user types something "tralala" and clicks Done/presses Enter, the new Line is saved & updated.
  • When the user clicks on an existing Line of Text, the Transcription dialog opens...
    • If the user clicks Close/presses Esc, the Line is unchanged.
    • If the user makes changes (where the text is NOT empty) "trololo" and clicks Done/presses Enter, the Line is updated.
    • If the user deletes the text "" and clicks Done/presses Enter, the Line is deleted. 👀

Most of these conditions are pretty straightforward/expected, but the conditions marked with 👀 are probably the ones you need to stare at and rub your chin thoughtfully at before giving a 👍 or 👎

That said, it may make more sense for me to 👍 this PR, merge it, and deploy it so you can see the changes in action. This PR is stable enough for a release. @wgranger Any issues with this? (EDIT: Wait, do we have staging? I vaguely remember that, or it might have been a Christmas dream.)

@shaunanoordin
Copy link
Member

PR Review (Update)

As mentioned, I'm going to 👍 and merge this PR. The changes themselves are stable enough to deploy, and I'd like to get these changes out to production so the team can see the UI/UX changes in action.

Status

PR is stable enough. Merging, deploying to production, with the expectation that further updates/tweaks (e.g. we still haven't resolved that Ctrl+A shortcut) will need to be introduced.

@shaunanoordin shaunanoordin merged commit 45f118b into master Jan 5, 2018
@wgranger wgranger deleted the keyboard-shortcuts branch January 5, 2018 17:46
@shaunanoordin
Copy link
Member

Wait... I need to get a sanity check on this. I'm proposing a deploy to production on a Friday afternoon. Taking this to #imls on Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaunanoordin shaunanoordin shaunanoordin approved these changes

Assignees

@shaunanoordin shaunanoordin

Labels
Second Beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wgranger @shaunanoordin @snblickhan @beckyrother